-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OHRI-1963 Enable editing of L&D form infant details in OHRI #1689
Conversation
@@ -82,6 +82,17 @@ export function getEstimatedDeliveryDate(patientUuid: string, pTrackerId: string | |||
}); | |||
} | |||
|
|||
export function getIdentifierInfo(identifier: string){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kajambiya this function looks solid to me but the only thing missing here is HTTP request & response error handling, would be good if you can add to this function and we show this to the other devs otherwise we will keep having those errors that emerge out of the blue and the user doesn't understand. Here in case an HTTP request or response error happens I would expect to see a message like Failed to retrieve identifier information. Also shouldn't this function be marked as async
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ebambo, All the error handling regarding api calls including http error handling is done in the openmrsFetch function
We don't need to mark the function async
because we do not care when it finishes processing. We would have to add async if there was need to await
for the operation to first finish.
export const getIdentifierAssignee = async (identifier: string, identifierType: string) => { | ||
return getIdentifierInfo(identifier).then((data) => { | ||
if (data) { | ||
for (const result of data.results) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if data
has no results
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment also applies to the lines below, you better explicitly check or add the question marks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, by the time we get here, all the other possible errors were checked and the call is fine. If there's no data, it only means that there's no patient with that identifier meaning it's ok to go ahead and assign it to the current patient.
// only do this the first time the form is entered | ||
if (sessionMode !== 'enter') { | ||
if (sessionMode === 'view') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment or delete it to indicate the hook runs on both enter and edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/esm-ohri-pmtct-app/src/post-submission-actions/mother-child-linkage-action.ts
Outdated
Show resolved
Hide resolved
if(existingpTrackerAssignee){ | ||
if(sessionMode === 'enter'){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this include edit mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted accordingly
} else { | ||
//In edit mode, only throw error if the patient with the existing PTracker is not linked with the current mother | ||
const parentRelationships = await fetchPatientRelationships(parent.id); | ||
const isAlreadyLinked = parentRelationships.some( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌🏿
Requirements
OHRI-123 My PR title
.Summary
Screenshots
Related Issue
Other